Add microgrid types#14
Conversation
flora-hofmann-frequenz
added
the
cmd:skip-release-notes
github-actions
Bot
added
the
part:microgrid
| ) | ||
|
|
||
| # Set up logging | ||
| _logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Is logging needed / useful / used in this module?
| from dataclasses import dataclass | ||
| from typing import List | ||
|
|
||
| # pylint: disable=import-error, no-name-in-module |
There was a problem hiding this comment.
I think no-name-in-module should be enough here. Also we want to enable it again after the import with
# pylint: enable=no-name-in-module
the CI error is related to mypy, not sure whats up there
| """Convert a protobuf MicrogridComponentIDs to MicrogridComponentIDs object. | ||
|
|
||
| Args: | ||
| microgrid_component_ids: MicrogridComponentIDs to convert. |
There was a problem hiding this comment.
missing blank line before Returns:
flora-hofmann-frequenz
force-pushed
the
add_microgrid
branch
from
67b0c67 to
aa25e3e
Compare
|
I made some similar comments in: I think we should focus on the review of only one and delay the review of the other after that, because I think there will be a lot of duplicate comments. Since I made quite a few comments to #15 and most comments done here are also covered there, I suggest to start with #15. I will make this a draft PR to make this clear. If you don't agree please feel free to change it back to a final PR. |
flora-hofmann-frequenz
force-pushed
the
add_microgrid
branch
from
aa25e3e to
5ed9e0a
Compare
github-actions
Bot
added
part:tests
Signed-off-by: Flora <flora.hofmann@frequenz.com>
flora-hofmann-frequenz
force-pushed
the
add_microgrid
branch
from
5ed9e0a to
bd68404
Compare
| """Frequenz microgrid definition.""" | ||
| """Module to define the microgrid used with the common client.""" | ||
|
|
||
| from __future__ import annotations # required for constructor type hinting |
There was a problem hiding this comment.
This shouldn't be necessary anymore if you use Self, right?
| """ | ||
| return cls( | ||
| microgrid_id=microgrid_component_ids.microgrid_id, | ||
| component_ids=List(microgrid_component_ids.component_ids), |
There was a problem hiding this comment.
This raises an interesting question, copying the list (or whatever component_ids is in the proto class) is definitely the safe option, but it means doing some copying, when in 99% of the cases you'll probably just take the protobuf message, convert it to the wrapper and discard it, so it is very unlikely it will be mutated after creating the wrapper (in which case it will also affect the wrapper list, as we stored a reference to the protobuf message).
Maybe since it is unlikely and might have a performance impact, we could document that a reference is taken and the users should take care of the copying if they plan to keep mutating the proto message instance.
@frequenz-floss/python-sdk-team any opinions?
There was a problem hiding this comment.
Haven't really figured out what this type is for, but it still looks like a one time thing that we might have to process at startup, and not along with each streaming message, etc. So I don't see any benefit from not choosing the conservative approach.
There was a problem hiding this comment.
Yeah, this is a more general question, because we will probably have it in other situations, as for this particular class, I guess we need to figure out if we want to pursue this PR as you mentioned in #8 (comment), I think it was a good point that we might be missing the end goal here.
There was a problem hiding this comment.
I agree it is fine to copy the IDs if the message is only processed once at startup (or only a few times). For general scenarios it makes sense to avoid the copy if it is possible
There was a problem hiding this comment.
And to @shsms point, if this is known to never leave the client, then we always know the protobuf is discarded and we don't need to copy. All points to not really having to copy stuff when converting from/to protobuf in this repo, again under the premise that types here will never be exposed to the user as is.
| return PBMicrogridComponentIDs( | ||
| microgrid_id=self.microgrid_id, | ||
| component_ids=List(self.component_ids), | ||
| ) |
There was a problem hiding this comment.
Same here, if the common case would just be: client.send(instance.to_proto()), then copying doesn't make a lot of sense.
Co-authored-by: Leandro Lucarella <luca@llucax.com> Signed-off-by: flora-hofmann-frequenz <88315331+flora-hofmann-frequenz@users.noreply.github.com>
Co-authored-by: Leandro Lucarella <luca@llucax.com> Signed-off-by: flora-hofmann-frequenz <88315331+flora-hofmann-frequenz@users.noreply.github.com>
Co-authored-by: Leandro Lucarella <luca@llucax.com> Signed-off-by: flora-hofmann-frequenz <88315331+flora-hofmann-frequenz@users.noreply.github.com>
Co-authored-by: Leandro Lucarella <luca@llucax.com> Signed-off-by: flora-hofmann-frequenz <88315331+flora-hofmann-frequenz@users.noreply.github.com>
| microgrid_id: int | ||
| """The ID of the microgrid.""" | ||
|
|
||
| component_ids: Sequence[int] |
There was a problem hiding this comment.
Sequence was not imported
There was a problem hiding this comment.
Actually the type here might depend on the resolution of https://github.com/frequenz-floss/frequenz-client-common-python/pull/14/files#r1495647444
|
Closed as this will also be changed in connection with reporting client mvp. |
5 participants
Split up of types modules to reflect protobuf structure.